Use HostContext to warn about invalid View/Text nesting #12766

bvaughn · 2018-05-08T16:40:27Z

React Native currently uses the context API to warn about invalid nesting of Text and View components. Some of this is currently necessary because of conditional logic like this. Other parts could be moved here though (like View warning if it's within a Text ancestor) in order to remove the necessity of using an extra Context.Consumer component. Planned changes to React Native may also allow for moving other Context.Consumer components from the JS side, which would make this warning approach more attractive.

cc @yungsters

gaearon · 2018-05-08T16:42:52Z

packages/react-native-renderer/src/ReactNativeFiberRenderer.js

+    parentHostContext: HostContext,
+    type: string,
+  ): HostContext {
+    return {type};


If you don't need this in PROD, let's avoid the extra allocation? Like ReactDOM does.

Same for the root one.

sebmarkbage · 2018-05-08T16:43:50Z

packages/react-native-renderer/src/ReactNativeFiberRenderer.js

+    parentHostContext: HostContext,
+    type: string,
+  ): HostContext {
+    return {type};


This is providing a new context object for every node in the tree. We should only do this in DEV and even in DEV we should only do this when we are switching between useful context differences (e.g. from non-text to text). In DOM we change the context only when we switch between HTML and SVG, not every node along the way.

Yah, hadn't yet pushed the DEV only check, but it's there now.

Interesting suggestion with the second bit. I'll add that.

sebmarkbage

All changes now needs to be replicated in the Fabric renderer too.

pull-bot · 2018-05-08T16:51:20Z

Details of bundled changes.

Comparing: fc3777b...2ca4555

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.2%	+0.2%	470 KB	470.75 KB	100.5 KB	100.72 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	🔺+0.3%	🔺+0.5%	225.13 KB	225.73 KB	37.67 KB	37.87 KB	RN_FB_PROD
ReactNativeRenderer-dev.js	+0.2%	+0.2%	469.73 KB	470.48 KB	100.44 KB	100.66 KB	RN_OSS_DEV
ReactNativeRenderer-prod.js	🔺+0.3%	🔺+0.6%	219.24 KB	219.84 KB	36.6 KB	36.81 KB	RN_OSS_PROD
ReactFabric-dev.js	+0.2%	+0.2%	452.39 KB	453.14 KB	96.08 KB	96.29 KB	RN_FB_DEV
ReactFabric-prod.js	🔺+0.3%	🔺+0.6%	204.88 KB	205.5 KB	34 KB	34.19 KB	RN_FB_PROD
ReactFabric-dev.js	+0.2%	+0.2%	452.42 KB	453.17 KB	96.09 KB	96.31 KB	RN_OSS_DEV
ReactFabric-prod.js	🔺+0.3%	🔺+0.6%	204.92 KB	205.54 KB	34.02 KB	34.21 KB	RN_OSS_PROD

Generated by 🚫 dangerJS

bvaughn · 2018-05-08T17:00:49Z

All changes now needs to be replicated in the Fabric renderer too.

Ah, right. Okay~ added to fabric too.

sebmarkbage

I think the flaw here is that currently it's not just a DEV only thing. Its native protocol is contextual. So even if you add this host context, you can't remove the context from the wrapper.

https://github.com/facebook/react-native/blob/master/Libraries/Text/Text.js#L218-L222

It renders different components depending on where you are. @shergin is fixing this on iOS Fabric to not be needed but currently it's needed on the Android side.

I think the solution to that is to fix the native side so that the difference between RCTVirtualText and RCTText isn't necessary to deal with in JS. Maybe. Although it might be nice to be able to optimize from JS.

sebmarkbage · 2018-05-08T16:58:39Z

packages/react-native-renderer/src/ReactNativeFiberRenderer.js

+    if (__DEV__) {
+      const oldIsInAParentText = parentHostContext.isInAParentText;
+      const newIsInAParentText =
+        type === 'RCTText' || type === 'RCTVirtualText';


A new parent can never be a RCTVirtualText. Because of this:

https://github.com/facebook/react-native/blob/master/Libraries/Text/Text.js#L218-L222

Which highlights that this is in fact not just a DEV only contextual thing.

A new parent can never be a RCTVirtualText.

I'm not sure I understand what you're saying here.

getChildHostContext() can get called with type === "RCTVirtualText" in cases like:

<Text> <Text>this is inside of a virtual text</Text> </Text>

Which highlights that this is in fact not just a DEV only contextual thing.

I don't understand. The "DEV only" bit here is the warning, not the runtime behavior.

bvaughn · 2018-05-08T17:26:01Z

I think the flaw here is that currently it's not just a DEV only thing. Its native protocol is contextual. So even if you add this host context, you can't remove the context from the wrapper.
I think the solution to that is to fix the native side so that the difference between RCTVirtualText and RCTText isn't necessary to deal with in JS...

Agreed, but I think the DEV warning about View not being allowed inside of a text, or string/text not being allowed outside of a Text is still valid. No? The Android-specific conditional will change it sounds like, but the general warning still seems useful.

Edit I think you're pointing out that even after these changes, the context API DEV bits in the e.g. Text.js component can't be removed- but we think it can eventually be deprecated. Same with TextInput. (Tim and I have chatted about this a bit in meatspace.)

sophiebits · 2018-05-08T17:27:35Z

packages/react-native-renderer/src/ReactFabricRenderer.js

+    type: string,
+  ): HostContext {
+    if (__DEV__) {
+      const oldIsInAParentText = parentHostContext.isInAParentText;


Why this way vs what you had before?

To avoid creating new wrapper objects if isInAParentText hasn't changed.

sebmarkbage · 2018-05-08T17:41:04Z

The issue is that if we still need the context in user space, what do we gain by adding it in the reconciler? Other than slightly more overhead.

What you could do is make this a prod context too and use it to swap to RCTVirtualText automatically in the renderer instead of in the wrapper. It's a bit unfortunate because we have to add conditionals that gets tested for every kind of view, not just text.

sebmarkbage · 2018-05-08T17:42:50Z

packages/react-native-renderer/src/ReactFabricRenderer.js

    type: string,
  ): HostContext {
    if (__DEV__) {
      const oldIsInAParentText = parentHostContext.isInAParentText;


We're only going in one direction. If oldIsInAParentText is true, you can bail out early.

That's fair.

Isn't Text > View > Text valid while Text > View > (plain text) isn't?

Text -> View is not valid.

(It currently is for iOS but it soon won't be. Tim is changing this now.)

cc @yungsters in case this is not correct

Wait why? What do you do when you need to put a view inline in some text?

Certain types of components (like Image) are allowed inside of Text - but RCTView itself (the <View> component) is not.

Hm. But I think I see what you're saying. Only treating this value as a one-way thing would suggest that this is allowed: <Text> -> <Image> -> "foo" (although this specific case would have a different error about Image not accepting children...)

Maybe this is a better example of why we shouldn't only set this value one way:

<Text> <ScrollView> foo </ScrollView> </Text>

I don't think this is valid. We should warn.

I'm going to add a test for this and back out the one-way change.

bvaughn · 2018-05-08T17:48:27Z

The issue is that if we still need the context in user space, what do we gain by adding it in the reconciler? Other than slightly more overhead.What you could do is make this a prod context too and use it to swap to RCTVirtualText automatically in the renderer instead of in the wrapper. It's a bit unfortunate because we have to add conditionals that gets tested for every kind of view, not just text.

Fair question! I pushed this kind of for discussion purposes more so than thinking it's definitely a good idea.

That being said, if you look at a diff like D7895382, we could remove the Context.Consumer from View (a component that probably appears many times in an app) if we didn't have to worry about DEV warning for nested <View> in <Text>. (Although I guess this is DEV only so maybe not a huge win.)

We could maybe also move the RCTVirtualText distinction and get rid of Context.Consumer in Text. (I think it would be nicer to remove this distinction from JavaScript entirely and just let the native side handle it- but I'm not sure if there are gotchas there.)

yungsters · 2018-05-09T05:57:27Z

I think the solution to that is to fix the native side so that the difference between RCTVirtualText and RCTText isn't necessary to deal with in JS. Maybe. Although it might be nice to be able to optimize from JS.

I don't think there is disagreement on mitigating the need to deal with this in JS. In fact, I was just discussing this with @shergin and @mdvacca today. With the new UIManager implementations and our desire to re-think some of the core primitives, I see this being only a matter of time.

The issue is that if we still need the context in user space, what do we gain by adding it in the reconciler?

This pull request actually adds two checks to the reconciler:

Disallow <View> inside of <Text>.
Disallow strings outside of <Text> and <TextInput>.

The first check is definitely redundant today and we do not gain anything from adding it. The only thing that we get is that once all the native components no longer require the use of context at runtime, we'll also be able to clean up the check from View.js (without adding this back to the reconciler, which is non-trivial for anyone except probably you and @bvaughn).

The second check is actually not possible today in user space (e.g. Text.js or View.js) and the primary motivator for this pull request. Components only have visibility into the composed elements in children. Without decomposing them into primitives, we cannot know whether every child of a <View> is not a string.

I hope this makes sense.

bvaughn · 2018-05-10T14:08:22Z

I hope this makes sense.

It did 😄

…nticipation of Tim removing this change on RN

bvaughn · 2018-05-10T22:58:25Z

I've removed warning in favor of console.error based on @yungsters's feedback. This way RN will show a redbox instead of a yellow box for this case.

yungsters

This is awesome. Thanks again for helping us make this much easier to debug in React Native.

yungsters · 2018-05-10T23:37:12Z

packages/react-native-renderer/src/ReactFabricRenderer.js


+type HostContext = {
+  isInAParentText: boolean,
+};


In React Native, we've started adopting a common practice to use:

type HostContext = $ReadOnly<{| isInAParentText: boolean, |}>;

This makes it so you cannot write to isInAParentText and disallows access of properties other than the ones supplied. But I'd rather you use what is common in the React repository.

Good point! I've been trying to use exact types when I think of it. I dig the $ReactOnly suggestion too.

yungsters · 2018-05-10T23:38:31Z

packages/react-native-renderer/src/ReactFabricRenderer.js

-  getRootHostContext(): {} {
-    return emptyObject;
+  getRootHostContext(rootContainerInstance: Container): HostContext {
+    return __DEV__ ? {isInAParentText: false} : emptyObject;


I'm surprised that emptyObject fulfills the type definition for HostContext. I would've expected isInAParentText to need to be optional (isInAParentText?: boolean), but maybe emptyObject is typed with something lossy like Object?

That's true. I'd guess emptyObject is resolving to an any type?

Actually something more insidious is going on here. I can't make Flow report an error in this type no matter what I do, within the React native or fabric renderers.

For example, given this type:

type HostContext = $ReadOnly<{| isInAParentText: boolean, |}>;

Flow reports "no errors" if I add these lines inside of a method like createInstance() that accepts a hostContext: HostContext param:

hostContext.isInAParentText = false;

But if I change it to this, Flow fails:

(hostContext: HostContext).isInAParentText = false;

Covariant property isInAParentText incompatible with contravariant use in assignment of property isInAParentText

Note that the React repo is using flow-cli v0.61 and the latest version is v0.72 ~ not sure if this could be an old Flow bug.

Resolution: I was trying to reproduce inside of a conditional, and we're using an older version of Flow that has a bug (reproducible in the REPL) that does not properly handle this case.

:o Should we upgrade Flow?

Yes. I'll add this discussion point to our sync notes.

Here is a failing case:

type HostContext = $ReadOnly<{| isInAParentText: boolean |}>; const hostContext: HostContext = { isInAParentText: true }; if (hostContext.thisPropertyDoesNotExist) { // And this one is a Boolean and should not be writable hostContext.isInAParentText = 'abc'; }

Flow 0.61.0 link

Flow 0.72.0 link

Addressed initial feedback.

bvaughn · 2018-05-11T16:12:59Z

Based on an in-person conversation yesterday with Tim and Kevin about future RN direction, I've replaced the DEV-only checks that logged to console.error with an invariant. I did this because the invalid nesting will cause a crash on the native side, and it would be better for there to be a meaningful error on the JavaScript side in this event.

Back to @yungsters and @sebmarkbage for review and consideration.

yungsters

Looks great. Thanks!

shergin · 2018-05-12T04:56:37Z

It renders different components depending on where you are. @shergin is fixing this on iOS Fabric to not be needed but currently it's needed on the Android side.
I think the solution to that is to fix the native side so that the difference between RCTVirtualText and RCTText isn't necessary to deal with in JS. Maybe. Although it might be nice to be able to optimize from JS.

I am afraid, it is used in Fabric, and I don't plan to remove it. (In the first version of this component in Fabric that condition looked a bit different, but now it is identical to Paper.)

Even if it is probably possible, it is far from trivial and probably overcomplicate several things on the native side. Text and VirtualText accept different sets of props and behaves differently; and conceptually they are completely different things.

Ideally, the <Text props> should be just a syntax sugar that works like that:

let {textViewProps, viewProps, textAttributesProps, children} = props;
if (nested) {
  return <TextAttributes textAttributesProps children />;
} else {
  return (
    <TextView textViewProps viewProps>
      <TextAttributes textAttributesProps children />
    </TextView>
  );
}

sophiebits · 2018-05-12T05:23:40Z

@shergin Sending both Text and VirtualText for a single non-nested Text is an easy change to make on the JS side you prefer the two separated on the native side.

shergin · 2018-05-14T22:27:51Z

Yeah, sure.
BTW, I am totally okay with the PR, I was talking about far and idealistic future.

bvaughn · 2018-05-14T22:34:34Z

Thanks all.

Once RN is ready on the native side for us to do more cleanup, let's do it! I'm happy to make more changes on the renderer side to enable this.

facebook-github-bot added the CLA Signed label May 8, 2018

gaearon reviewed May 8, 2018

View reviewed changes

sebmarkbage previously requested changes May 8, 2018

View reviewed changes

sebmarkbage reviewed May 8, 2018

View reviewed changes

sophiebits reviewed May 8, 2018

View reviewed changes

sebmarkbage reviewed May 8, 2018

View reviewed changes

Brian Vaughn added 12 commits May 10, 2018 15:47

Use HostContext to warn about invalid view/text nesting

c9c1ca6

Added tests for HostContext warning behavior

fcf2f7a

Return host context in DEV only

97709b5

Fixed ReactNativeEvents

e0bd283

Simplified HostContext; dont recreate unless value has changed

3b214bc

Added same HostContext changes to fabric renderer

61d1067

Account for TextInput as well

9e8850f

Account for Images inside of Text. Remove Android-specific check in a…

f0f1db2

…nticipation of Tim removing this change on RN

Updated warning in test

c1c0805

Bailout earlier when isInAParentText true

4821bf8

Added a test for <Text><ScrollingView>foo case

b810501

Replaced warning() with console.error()

079e4ea

bvaughn force-pushed the rn-text branch from de876a1 to 079e4ea Compare May 10, 2018 22:53

yungsters approved these changes May 10, 2018

View reviewed changes

Brian Vaughn added 2 commits May 11, 2018 09:03

Made native/fabric HostContext type more explicit

2cab4bf

Replace DEV Text/View nesting check with invariant

a1c01b8

Added an inline comment

2ca4555

yungsters approved these changes May 11, 2018

View reviewed changes

bvaughn merged commit c802d29 into facebook:master May 14, 2018

bvaughn deleted the rn-text branch May 14, 2018 22:34

Use HostContext to warn about invalid View/Text nesting #12766

Use HostContext to warn about invalid View/Text nesting #12766

Uh oh!

Conversation

bvaughn commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage left a comment

Choose a reason for hiding this comment

Uh oh!

pull-bot commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

react-native-renderer

Uh oh!

bvaughn commented May 8, 2018

Uh oh!

sebmarkbage left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yungsters commented May 9, 2018

Uh oh!

bvaughn commented May 10, 2018

Uh oh!

bvaughn commented May 10, 2018

Uh oh!

yungsters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bvaughn commented May 8, 2018 •

edited

Loading

bvaughn May 8, 2018 •

edited

Loading

pull-bot commented May 8, 2018 •

edited

Loading

sebmarkbage left a comment •

edited

Loading

bvaughn commented May 8, 2018 •

edited

Loading

bvaughn May 8, 2018 •

edited

Loading

bvaughn May 8, 2018 •

edited

Loading

bvaughn commented May 8, 2018 •

edited

Loading